Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang] AliasAnalysis: distinguish addr of arg vs. addr in arg #87723

Closed
wants to merge 6 commits into from

Conversation

jdenny-ornl
Copy link
Collaborator

For example, in the following program, the relationship between the address in arg (dynamically the address of x's alloca) and the address of arg (dynamically the address of p's alloca) is not MustAlias, as determined without this patch:

subroutine f()
  real, pointer :: p
  real, target :: x
  p => x
  call g(p)
end subroutine f
subroutine g(arg)
  real, pointer :: arg
end subroutine g

Generally extend test coverage for HLFIR-based alias analysis for addresses extracted from pointers.

For example, in the following program, the relationship between the
address *in* arg (dynamically the address of x's alloca) and the
address *of* arg (dynamically the address of p's alloca) is not
MustAlias, as determined without this patch:

```
subroutine f()
  real, pointer :: p
  real, target :: x
  p => x
  call g(p)
end subroutine f
subroutine g(arg)
  real, pointer :: arg
end subroutine g
```

Generally extend test coverage for HLFIR-based alias analysis for
addresses extracted from pointers.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Apr 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 4, 2024

@llvm/pr-subscribers-flang-fir-hlfir

Author: Joel E. Denny (jdenny-ornl)

Changes

For example, in the following program, the relationship between the address in arg (dynamically the address of x's alloca) and the address of arg (dynamically the address of p's alloca) is not MustAlias, as determined without this patch:

subroutine f()
  real, pointer :: p
  real, target :: x
  p => x
  call g(p)
end subroutine f
subroutine g(arg)
  real, pointer :: arg
end subroutine g

Generally extend test coverage for HLFIR-based alias analysis for addresses extracted from pointers.


Full diff: https://github.com/llvm/llvm-project/pull/87723.diff

4 Files Affected:

  • (modified) flang/include/flang/Optimizer/Analysis/AliasAnalysis.h (+2-2)
  • (modified) flang/lib/Optimizer/Analysis/AliasAnalysis.cpp (+15-5)
  • (modified) flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir (+1-1)
  • (added) flang/test/Analysis/AliasAnalysis/alias-analysis-box-addr-load/arg.fir (+90)
diff --git a/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h b/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
index dfcafe88fee1b5..dd4eb363cc7277 100644
--- a/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
+++ b/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
@@ -36,8 +36,8 @@ struct AliasAnalysis {
              /// Represents memory allocated outside of a function
              /// and passed to the function via host association tuple.
              HostAssoc,
-             /// Represents direct memory access whose source cannot be further
-             /// determined
+             /// Memory address retrieved from a box, perhaps from a global or
+             /// an argument.
              Direct,
              /// Represents memory allocated by unknown means and
              /// with the memory address defined by a memory reading
diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index e144640081cbf3..f0af0dd547f980 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -91,8 +91,14 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
   LLVM_DEBUG(llvm::dbgs() << "AliasAnalysis::alias\n";
              llvm::dbgs() << "  lhs: " << lhs << "\n";
              llvm::dbgs() << "  lhsSrc: " << lhsSrc << "\n";
+             llvm::dbgs() << "  lhsSrc kind: " << EnumToString(lhsSrc.kind) << "\n";
+             llvm::dbgs() << "  lhsSrc pointer: " << lhsSrc.attributes.test(Attribute::Pointer) << "\n";
+             llvm::dbgs() << "  lhsSrc target: " << lhsSrc.attributes.test(Attribute::Target) << "\n";
              llvm::dbgs() << "  rhs: " << rhs << "\n";
              llvm::dbgs() << "  rhsSrc: " << rhsSrc << "\n";
+             llvm::dbgs() << "  rhsSrc kind: " << EnumToString(rhsSrc.kind) << "\n";
+             llvm::dbgs() << "  rhsSrc pointer: " << rhsSrc.attributes.test(Attribute::Pointer) << "\n";
+             llvm::dbgs() << "  rhsSrc target: " << rhsSrc.attributes.test(Attribute::Target) << "\n";
              llvm::dbgs() << "\n";);
 
   // Indirect case currently not handled. Conservatively assume
@@ -101,8 +107,10 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
     return AliasResult::MayAlias;
   }
 
-  // SourceKind::Direct is set for the addresses wrapped in a global boxes.
-  // ie: fir.global @_QMpointersEp : !fir.box<!fir.ptr<f32>>
+  // SourceKind::Direct is set for the addresses wrapped in a box, perhaps from
+  // a global or an argument.
+  // e.g.: fir.global @_QMpointersEp : !fir.box<!fir.ptr<f32>>
+  // e.g.: %arg0: !fir.ref<!fir.box<!fir.ptr<f32>>>
   // Though nothing is known about them, they would only alias with targets or
   // pointers
   bool directSourceToNonTargetOrPointer = false;
@@ -399,16 +407,18 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
   if (!defOp && type == SourceKind::Unknown)
     // Check if the memory source is coming through a dummy argument.
     if (isDummyArgument(v)) {
-      type = SourceKind::Argument;
       ty = v.getType();
       if (fir::valueHasFirAttribute(v, fir::getTargetAttrName()))
         attributes.set(Attribute::Target);
-
       if (Source::isPointerReference(ty))
         attributes.set(Attribute::Pointer);
+      if (followBoxAddr && attributes.test(Attribute::Pointer))
+        type = SourceKind::Direct;
+      else
+        type = SourceKind::Argument;
     }
 
-  if (type == SourceKind::Global || type == SourceKind::Direct)
+  if (global)
     return {global, type, ty, attributes, approximateSource};
 
   return {v, type, ty, attributes, approximateSource};
diff --git a/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir b/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir
index 31459ef21d947c..d915ba9d27905a 100644
--- a/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir
+++ b/flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir
@@ -44,7 +44,7 @@
 // pointer arguments
 // CHECK-DAG: arg2.addr#0 <-> func.region0#0: MayAlias
 // CHECK-DAG: arg2.addr#0 <-> func.region0#1: MayAlias
-// CHECK-DAG: arg2.addr#0 <-> func.region0#2: MustAlias
+// CHECK-DAG: arg2.addr#0 <-> func.region0#2: MayAlias
 // CHECK-DAG: boxp1.addr#0 <-> arg2.addr#0: MayAlias
 
 func.func @_QFPtest(%arg0: !fir.ref<f32> {fir.bindc_name = "v1", fir.target}, %arg1: !fir.ref<f32> {fir.bindc_name = "v2", fir.target}, %arg2: !fir.ref<!fir.box<!fir.ptr<f32>>> ) attributes {test.ptr = "func"} {
diff --git a/flang/test/Analysis/AliasAnalysis/alias-analysis-box-addr-load/arg.fir b/flang/test/Analysis/AliasAnalysis/alias-analysis-box-addr-load/arg.fir
new file mode 100644
index 00000000000000..de1b478fa9599e
--- /dev/null
+++ b/flang/test/Analysis/AliasAnalysis/alias-analysis-box-addr-load/arg.fir
@@ -0,0 +1,90 @@
+// Check aliasing with the address passed via a pointer dummy argument.
+
+// Use --mlir-disable-threading so that the AA queries are serialized
+// as well as its diagnostic output.
+// RUN: fir-opt %s \
+// RUN:   -pass-pipeline='builtin.module(func.func(test-fir-alias-analysis))' \
+// RUN:   --mlir-disable-threading 2>&1 | FileCheck %s
+
+// subroutine test(p0, p1, arr, t_arr, alloc, t_alloc)
+//   real, pointer :: p0, p1
+//   real :: arr(:)
+//   real, target :: t_arr(:)
+//   real, allocatable :: alloc
+//   real, allocatable, target :: t_alloc
+//   real, target :: t
+//   real :: v
+//   v = p0
+//   v = p1
+//   v = arr(1)
+//   v = t_arr(1)
+//   v = alloc
+//   v = t_alloc
+// end subroutine test
+
+// CHECK-LABEL: Testing : "_QPtest"
+
+// The address in a pointer can alias the address in another pointer or the
+// address of a target but not the address of other variables.
+// CHECK-DAG: t.addr#0 <-> p0.tgt_addr#0: MayAlias
+// CHECK-DAG: t.addr#0 <-> p1.tgt_addr#0: MayAlias
+// CHECK-DAG: v.addr#0 <-> p0.tgt_addr#0: NoAlias
+// CHECK-DAG: v.addr#0 <-> p1.tgt_addr#0: NoAlias
+// CHECK-DAG: p0.tgt_addr#0 <-> p1.tgt_addr#0: MayAlias
+
+// Determining whether the address *in* a pointer can alias the address *of* a
+// pointer is not yet handled.  In the past, when it was the same pointer, that
+// relationship was mistakenly determined to be MustAlias.
+// CHECK-DAG: p0.tgt_addr#0 <-> func.region0#0: MayAlias
+// CHECK-DAG: p0.tgt_addr#0 <-> func.region0#1: MayAlias
+// CHECK-DAG: p1.tgt_addr#0 <-> func.region0#0: MayAlias
+// CHECK-DAG: p1.tgt_addr#0 <-> func.region0#1: MayAlias
+
+// For some cases, AliasAnalysis analyzes hlfir.designate like fir.box_addr, so
+// make sure it doesn't mistakenly see arr(1).addr as an address that was loaded
+// from a pointer and that could alias something.  However, t_arr is a target.
+// CHECK-DAG: p0.tgt_addr#0 <-> arr(1).addr#0: NoAlias
+// CHECK-DAG: p0.tgt_addr#0 <-> t_arr(1).addr#0: MayAlias
+
+// Like a pointer, an allocatable contains an address, but an allocatable is not
+// a pointer and so cannot alias pointers.  However, t_alloc is a target.
+// CHECK-DAG: p0.tgt_addr#0 <-> alloc.tgt_addr#0: NoAlias
+// CHECK-DAG: p0.tgt_addr#0 <-> t_alloc.tgt_addr#0: MayAlias
+
+func.func @_QPtest(%arg0: !fir.ref<!fir.box<!fir.ptr<f32>>> {fir.bindc_name = "p0"}, %arg1: !fir.ref<!fir.box<!fir.ptr<f32>>> {fir.bindc_name = "p1"}, %arg2: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "arr"}, %arg3: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "t_arr", fir.target}, %arg4: !fir.ref<!fir.box<!fir.heap<f32>>> {fir.bindc_name = "alloc"}, %arg5: !fir.ref<!fir.box<!fir.heap<f32>>> {fir.bindc_name = "t_alloc", fir.target}) attributes {test.ptr="func"} {
+  %0:2 = hlfir.declare %arg4 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFtestEalloc"} : (!fir.ref<!fir.box<!fir.heap<f32>>>) -> (!fir.ref<!fir.box<!fir.heap<f32>>>, !fir.ref<!fir.box<!fir.heap<f32>>>)
+  %1:2 = hlfir.declare %arg2 {uniq_name = "_QFtestEarr"} : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, !fir.box<!fir.array<?xf32>>)
+  %2:2 = hlfir.declare %arg0 {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFtestEp0"} : (!fir.ref<!fir.box<!fir.ptr<f32>>>) -> (!fir.ref<!fir.box<!fir.ptr<f32>>>, !fir.ref<!fir.box<!fir.ptr<f32>>>)
+  %3:2 = hlfir.declare %arg1 {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFtestEp1"} : (!fir.ref<!fir.box<!fir.ptr<f32>>>) -> (!fir.ref<!fir.box<!fir.ptr<f32>>>, !fir.ref<!fir.box<!fir.ptr<f32>>>)
+  %4 = fir.alloca f32 {bindc_name = "t", fir.target, uniq_name = "_QFtestEt"}
+  %5:2 = hlfir.declare %4 {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFtestEt", test.ptr="t.addr"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+  %6:2 = hlfir.declare %arg5 {fortran_attrs = #fir.var_attrs<allocatable, target>, uniq_name = "_QFtestEt_alloc"} : (!fir.ref<!fir.box<!fir.heap<f32>>>) -> (!fir.ref<!fir.box<!fir.heap<f32>>>, !fir.ref<!fir.box<!fir.heap<f32>>>)
+  %7:2 = hlfir.declare %arg3 {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFtestEt_arr"} : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, !fir.box<!fir.array<?xf32>>)
+  %8 = fir.alloca f32 {bindc_name = "v", uniq_name = "_QFtestEv"}
+  %9:2 = hlfir.declare %8 {uniq_name = "_QFtestEv", test.ptr="v.addr"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+  %10 = fir.load %2#0 : !fir.ref<!fir.box<!fir.ptr<f32>>>
+  %11 = fir.box_addr %10 {test.ptr="p0.tgt_addr"} : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
+  %12 = fir.load %11 : !fir.ptr<f32>
+  hlfir.assign %12 to %9#0 : f32, !fir.ref<f32>
+  %13 = fir.load %3#0 : !fir.ref<!fir.box<!fir.ptr<f32>>>
+  %14 = fir.box_addr %13 {test.ptr="p1.tgt_addr"} : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
+  %15 = fir.load %14 : !fir.ptr<f32>
+  hlfir.assign %15 to %9#0 : f32, !fir.ref<f32>
+  %c1 = arith.constant 1 : index
+  %16 = hlfir.designate %1#0 (%c1) {test.ptr="arr(1).addr"} : (!fir.box<!fir.array<?xf32>>, index) -> !fir.ref<f32>
+  %17 = fir.load %16 : !fir.ref<f32>
+  hlfir.assign %17 to %9#0 : f32, !fir.ref<f32>
+  %c1_0 = arith.constant 1 : index
+  %18 = hlfir.designate %7#0 (%c1_0) {test.ptr="t_arr(1).addr"} : (!fir.box<!fir.array<?xf32>>, index) -> !fir.ref<f32>
+  %19 = fir.load %18 : !fir.ref<f32>
+  hlfir.assign %19 to %9#0 : f32, !fir.ref<f32>
+  %20 = fir.load %0#0 : !fir.ref<!fir.box<!fir.heap<f32>>>
+  %21 = fir.box_addr %20 {test.ptr="alloc.tgt_addr"} : (!fir.box<!fir.heap<f32>>) -> !fir.heap<f32>
+  %22 = fir.load %21 : !fir.heap<f32>
+  hlfir.assign %22 to %9#0 : f32, !fir.ref<f32>
+  %23 = fir.load %6#0 : !fir.ref<!fir.box<!fir.heap<f32>>>
+  %24 = fir.box_addr %23 {test.ptr="t_alloc.tgt_addr"} : (!fir.box<!fir.heap<f32>>) -> !fir.heap<f32>
+  %25 = fir.load %24 : !fir.heap<f32>
+  hlfir.assign %25 to %9#0 : f32, !fir.ref<f32>
+  return
+}

@jdenny-ornl
Copy link
Collaborator Author

I am still learning (HL)FIR and the AliasAnalysis implementation. Proposing this patch seems like the easiest way to ask about a test result in flang/test/Analysis/AliasAnalysis/alias-analysis-2.fir that doesn't make sense to me. I believe this patch improves that result. I would appreciate help from reviewers in understanding intended behavior if this patch breaks it.

Copy link

github-actions bot commented Apr 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

if (Source::isPointerReference(ty))
attributes.set(Attribute::Pointer);
if (followBoxAddr && attributes.test(Attribute::Pointer))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good overall. The test change looks correct.
Why do we need this condition : && attributes.test(Attribute::Pointer) ?
It should be symmetric with the AddrOfOp.

Copy link
Collaborator Author

@jdenny-ornl jdenny-ornl Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this isDummyArg() case: The pointer check is needed to exclude dummy arguments of type !fir.box<!fir.array<...>>. This is in part because followBoxAddr is set when DesignateOp is applied to a !fir.box.

For the AddrOfOp case: I didn't figure out how to get flang to generate a global with that type, even after asking in flang slack #general. I did find that flang/test/Fir/cg-ops.fir has such a global (again, I don't know the corresponding fortran). However, AddrOfOp turns even that into a !fir.ref, so followBoxAddr isn't set at the DesignateOp, so I still don't know how to create the problem for AddrOfOp.

Copy link
Collaborator Author

@jdenny-ornl jdenny-ornl Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the AddrOfOp case: I noticed that flang/test/Analysis/AliasAnalysis/alias-analysis-8.fir expects the relationship between the address in an allocatable global and the address of that allocatable global to be NoAlias.

For the isDummyArg() case: I just checked the same relationship for allocatable arguments and discovered my patch wasn't handling it correctly: it has it as MustAlias. The pointer check we're discussing is too strict. I'll push a patch to this PR shortly that relaxes that to fir::isa_ref_type(ty) to turn it into NoAlias. However, flang/test/Transforms/tbaa2.fir then fails because AddAliasTags cannot yet handle Direct for the allocatable arguments there. My new patch also attempts to extend AddAliasTags accordingly.

I also tried adding fir::isa_ref_type(ty) to the AddrOfOp case, and check-flang still behaved. So, it doesn't improve existing test cases, and I haven't found a !fir.box<!fir.array<...>> case for AddrOfOp (see previous comment), so for now I've abandoned that addition as apparently useless.

The address in it should be a `SourceKind::Direct` not a
`SourceKind::Argument`.  The fix is to set `SourceKind::Direct` in the
case of `!fir.heap` besides just pointers.

An effect is that the address *in* it cannot alias the address *of*
it.  flang/test/Analysis/AliasAnalysis/alias-analysis-8.fir expects
the same relationship in the case of allocatable globals.

Extend AddAliasTags to handle the case of `SourceKind::Direct` for an
arg, and update the occurrence in flang/test/Transforms/tbaa2.fir.
Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I don't see any regressions or bugs running a few benchmarks on my end. Please wait for another approval before merging.

if (Source::isPointerReference(ty))
attributes.set(Attribute::Pointer);
if (followBoxAddr && fir::isa_ref_type(ty))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe, this might be an attempt at handling INTENT(IN). With INTENT(IN) the box is passed by value so there is no !fir.ref type. Could we check for that instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand your meaning. Given the following:

subroutine test(p)
  real, pointer, intent(in) :: p
end subroutine test

flang-new generates:

func.func @_QPtest(%arg0: !fir.ref<!fir.box<!fir.ptr<f32>>> {fir.bindc_name = "p"}) {
  %0:2 = hlfir.declare %arg0 {fortran_attrs = #fir.var_attrs<intent_in, pointer>, uniq_name = "_QFtestEp"} : (!fir.ref<!fir.box<!fir.ptr<f32>>>) -> (!fir.ref<!fir.box<!fir.ptr<f32>>>, !fir.ref<!fir.box<!fir.ptr<f32>>>)
  return
}

If I drop the intent(in), the only difference is that the intent_in attribute is lost.

Would you please provide an example of the case you have in mind? Thanks.

Copy link
Collaborator Author

@jdenny-ornl jdenny-ornl Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove the fir::isa_ref_type(ty) check, several tests fail. For example, in the function test3 in flang/test/Analysis/AliasAnalysis/alias-analysis-host-assoc.fir, y(1) is then analyzed as a Direct instead of an Argument. It has type !fir.box<!fir.array<?xi32>> and is declared as integer, target :: y(:). Is that the information you're looking for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further thought, maybe there is a cleaner way to write this check:

  • Declare a new followBoxAddrLoad variable local to AliasAnalysis::getSource, and set it to true right before the return in the fir::LoadOp case.
  • For the dummy arg check above, replace followBoxAddr && fir::isa_ref_type(ty) with followBoxAddrLoad.
  • For the fir::AddrOfOp check, replace followBoxAddr && mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(ty)) with followBoxAddrLoad.

I tried that, and check-flang still succeeded. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in the process of compiling your changes and will give them a better look today. What you describe though sounds really good. I will give it a shot.

Copy link
Collaborator Author

@jdenny-ornl jdenny-ornl May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That test doesn't deal with a fortran pointer, so this patch leaves it as SourceKind::Argument

fir::isa_ref_type(ty) does not mean fortran POINTER. fir::isPointerType(ty) does.

Right, when I wrote that comment, I somehow forgot that I generalized beyond pointers. See my previous comment.

What additional benefits do you mean?

Boxes are also passed by value. You would want to distinguish between them and their data as well. In this case followBoxAddrLoad would not be set.

While the cases I'm trying to address so far specifically involve the fir::LoadOp case in AliasAnalysis::getSource, I'd appreciate any fortran example that does not. My understanding of relevant use cases is probably too narrow. [edit: Oops, you just posted one while I was writing this.]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it will be more clear to only keep the first four source kinds and move Direct into the attributes. We are looking for the source entity here, so the source kind should represent this source entity. Whatever happens during the use-def chain walk should not affect the kind of the source entity that we end up finding. The Direct, in this case, would mean that the reference value passed to getSource is a result of one or more dereferences of the original source. I just find it easier to think about it this way, though, you may know cases where it can complicate the implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was due for an example of box, passed by value:

subroutine test(p)
  real [,TARGET] :: p(:)
end subroutine test
func.func @_QPtest(%arg0: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "p", fir.target})

I am not the expert here (that would be @jeanPerier ), but the rationale was to communicate in FIR that the callee does not modify the box and that therefore, to the caller, the box is a shallow constant. They do end up being passed by reference in codegen but show as values in FIR.

Thanks for the example. That helps. However, I'm struggling with the idea of performing a FIR alias analysis on something (the box in this example) that FIR represents as a value not an address. If indeed it never makes sense to do that, then it seems it wouldn't matter whether we distinguish this box from its content as only the content is a candidate for alias analysis.

I wonder if it will be more clear to only keep the first four source kinds and move Direct into the attributes. We are looking for the source entity here, so the source kind should represent this source entity. Whatever happens during the use-def chain walk should not affect the kind of the source entity that we end up finding. The Direct, in this case, would mean that the reference value passed to getSource is a result of one or more dereferences of the original source. I just find it easier to think about it this way, though, you may know cases where it can complicate the implementation.

Might make sense. However, I've been assuming that, once you extract an address from a box, then you no longer have a global, dummy arg, local, or whatever the box is, so I thought it was ok to just call it a Direct. Maybe that's wrong? On that point, does anyone object to this patch's changes to flang/test/Transforms/tbaa2.fir? There, this patch causes the address extracted from a dummy argument allocatable to be labeled SourceKind::Direct and not SourceKind::Argument. Is that problematic?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling with the idea of performing a FIR alias analysis on something (the box in this example) that FIR represents as a value not an address

Right, we could do some ground work before adding further enhancements to enforce that we start from a memory reference always. I wanted to check with @jeanPerier before we do that, in case he might need to make adjustments in lowering.

Let's note that when we are at the LLVM level, the box is an address and it becomes a legitimate question to ask. I will have to dig some to see how we handle this case.

What I would be struggling with is that following the address of a POINTER takes you to a SourceKind::Direct but following the address of a TARGET takes you to a SourceKind::Argument.

I wonder if it will be more clear to only keep the first four source kinds and move Direct into the attributes

Yes, then we would get consistent source kinds for target and pointers and we should get the same attribute, that I would hope we would have renamed to something other than "Direct" then. That would require some buy-in from @tblah as it would require changes in runOnAliasInterface though it should not cause any change in the results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talking to Jean, reminded me that we are already handling the query on a box value as a query on the data.
bool followBoxAddr{mlir::isa<fir::BaseBoxType>(v.getType())};
We initialize followBoxAddr to true if we are starting from a box.

Though this is a moot point for this discussion since you are interested on a distinction between references.

I am working on a proposal to eliminate SourceKind::Direct while still retaining the information it was capturing. You still will be able to distinguish between data and box. I will write up an RFC tomorrow and will send out a WIP PR.

Renaud-K added a commit that referenced this pull request May 16, 2024
…ata and non-data (#91020)

This PR is an implementation for changes proposed in
https://discourse.llvm.org/t/rfc-distinguish-between-data-and-non-data-in-fir-alias-analysis/78759

Test updates were made when the query was on the wrong reference. So, it
is my hope that this will clear ambiguity on the nature of the queries
from here on.
There are also some TODOs that were addressed. 

It also partly implements what
#87723 is attempting to
accomplish. At least, on a point-to-point query between references, the
distinction is made. To apply it to TBAA, would be another PR.

Note that, the changes were minimal in the TBAA code to retain the
current results.
@jdenny-ornl
Copy link
Collaborator Author

PR #91020 replaced the important parts of this PR for my purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants